-
Notifications
You must be signed in to change notification settings - Fork 1.3k
restructure actions to reusable build flow with parameters #1779
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Just fair warning: GitHub Actions workflows changes tend to be really good when complete, and the biggest pain to get right, because you often cannot tell if it worked fully until post-merge. I am hoping it won't be too difficult here. |
|
BTW, while we are doing this, I see that multilib is disabled. If I already am doing this, do you want a parameter that disables any set of workflows? We could do something simple like (pseudocode): I like the idea, but am asking before adding it into the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR restructures the GitHub Actions workflows to use a single reusable build workflow with parameters, consolidating previously duplicated build logic across multiple workflows.
- Introduces a new reusable workflow (
build-reusable.yaml) that accepts parameters for OS, mode, target, compiler, and simulator options - Replaces individual build configurations in
build.yamlandnightly-release.yamlwith calls to the reusable workflow - Simplifies the nightly release process by using a modern GitHub release action and local git operations instead of API calls
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.github/workflows/build-reusable.yaml |
New reusable workflow that consolidates build logic with configurable matrix parameters |
.github/workflows/build.yaml |
Simplified to call the reusable workflow with appropriate parameters for different build types |
.github/workflows/nightly-release.yaml |
Updated activity check logic and release creation using modern actions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
180bef7 to
d4f4f53
Compare
|
Good copilot catches, fixed. |
|
I saw the build failed. I'll dig into it early next week. |
|
Might this PR also fortuitously fix this issue or does that need separate attention? |
|
Not sure. Once I get this working correctly, I don't mind looking at that. What sends out the emails? |
Thanks.
I thought that it was some standard part of the GitHub action? I looked into this issue a little a while back but can't remember the details unfortunately. |
d4f4f53 to
84b81ed
Compare
|
OK, figured out that particular failure, fixed and pushed. It needs approval to rerun the workflow. It was nice to see that the basic build jobs all ran correctly, just |
|
Any thoughts on the |
I don't see anything in the CI jobs that sends an email. There only are two CI workflows:
Looking at the latter (the only real candidate), it is set to
There is nothing there about sending out an email, so it is not something generic. It is possible that you as maintainers, as well as anyone else fully following the repository, automatically gets emails for any new release. I know I get those for other projects/repos that I maintain. Given how this is structured - first, create the release (which should include source automatically), then, in a separate job in the workflow, upload the assets - that the email is getting triggered by the first before the second is done, hence including just the source code assets. If so, switching to the actions-gh-release, which includes all of the assets, might successfully fix that. It depends on how the action does it under the covers. That does make sense, but cannot be proven until it is merged in. I would recommend waiting until this is merged in, and seeing what happens with first midnight post-merge. If it fixes it, great, we know that was the issue; if not, can dig deeper or elsewhere. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last comment from copilot seems not
84b81ed to
ad54d04
Compare
|
Fixes the raised issues. Could use another workflow run. |
|
This is an improvement. All of the runs are running. It is failing solely because of conflict naming in uploaded artifacts. I should be able to figure those out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ad54d04 to
db25153
Compare
Signed-off-by: Avi Deitcher <[email protected]>
db25153 to
ff49d85
Compare
|
Fixed the copilot nits. Also I believe fixed the artifact naming issue. Ready for another workflow run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| - name: Download Built Artifacts | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| path: ${{ env.ARTIFACTS_DIR }} |
Copilot
AI
Oct 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ARTIFACTS_DIR environment variable is not defined at this point. It should be moved from the create-release job to the top-level env section or defined before this step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one nit from me, otherwise LGTM
| shell: bash | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems adding trailing spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix that, but I want to let the build run to go through first, make sure there are no other issues.
|
This looks pretty good, all of the builds that were supposed to run, did so. The remaining thing is the nightly. It only kicks off midnight, and only if there were changes in the last 24 hours (which there will be after the merge, by definition). As soon as you merge, I will check the next morning, and if there are any issues, I will open a PR to fix them. Please don't merge on a Friday, as I won't see it on Saturday morning. I also will be out of touch this coming Tuesday, so please don't merge this coming Monday. |
|
@kito-cheng @cmuellner anything else I need to do here to make it good? |
|
OK I can check post-merge over the coming days. When can we move in, or what is missing? |
My 🤦♂️ mistake? 😆 Thanks. I am putting in my calendar to check this out shortly after midnight, see if anything in nightly breaks, so I can open fixes if needed. |
|
The nightly release job failed, but due to secondary directory permissions. I will figure it out and add a PR shortly. The part I really was concerned about - the update to checking activity in the last 24 hours, ensuring it found and downloaded all of the artifacts - all worked correctly, which is nice. |
|
Ah that explains it. We tried to download to |
|
OK got it. Opening a new PR. |
|
See #1787 |
Here is a first cut of #1778, as discussed with @cmuellner. I will describe what it does here. If you think that warrants a README inside the
.github/workflows/directory, happy to oblige.Before, we had multiple build runs, each redoing the same steps. They occurred in:
build.yml): (full matrix of variants)build.yml): just one variant, but with the--with-simargbuild.yml): two variantsnightly-release.yml): full matrix of variantsIn addition, one created releases, some created artifacts, some did not
This restructures it as follows.
First, there is a single reusable workflow, defined in
build-reusable.yml. It can get called by any other workflow. It has multiple args:--with-simThe other workflows - regular build, multilib, test-sim, nightly - all call it with the correct args.
In addition, I did a few more "cleanup" changes.
The reusable build and parameters make it much easier to test variants. You even could dispatch a build manually using a different set of parameters for the matrix, e.g. build on a different OS based or different compiler (I have no idea which one, but if you can think of one).
Looking forward to your feedback.